-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Make peers sync secondary index #2390
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2390 +/- ##
===========================================
+ Coverage 74.99% 75.00% +0.01%
===========================================
Files 268 268
Lines 25912 26003 +91
===========================================
+ Hits 19431 19502 +71
- Misses 5170 5177 +7
- Partials 1311 1324 +13
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I think there are few bits that need sorting out before merge
ef94bf8
to
ed5ec47
Compare
CreateDocIndex(context.Context, *Document) error | ||
|
||
// UpdateDocIndex updates the index for the given document. | ||
// WARNING: This method is only for internal use and is not supposed to be called by the client |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: These are suitably scary - thanks 😁
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I second that 👍
if err != nil { | ||
return err | ||
} | ||
err = c.indexNewDoc(ctx, txn, newDoc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I do see delete+new
as still being business logic, but only at a nitpick level - so feel free to merge as is :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you expand on what is your nitpick concern here Andy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have none and thought I was in the txn_db.go file :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see I how I got confused - they have moved to collection, and I new that they had moved, but my brain still thought of the implementation as being in the txn_db file when I saw the txn logic. I wonder if we can do a similar job to collection that we did to client.Store and split out the txn boilerplate to another file sometime
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
return nil, client.NewErrCollectionNotFoundForSchema(schemaRoot) | ||
} | ||
var col client.Collection | ||
for _, c := range cols { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
praise: Thanks for moving this somewhere out of the way :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - thanks Islam :)
Relevant issue(s)
Resolves #2379
Description
Make peers updated update local storage when an update happens on docs.